Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explicit cancellation of streams on client exceptions #211

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Explicit cancellation of streams on client exceptions #211

merged 1 commit into from
Sep 5, 2024

Conversation

FinleyMcIlwaine
Copy link
Collaborator

@FinleyMcIlwaine FinleyMcIlwaine commented Aug 23, 2024 •
edited
Loading

Explicitly cancel streams upon exiting the scope of withRPC. If the stream terminates normally before exiting withRPC, our changes to http2 guarantee that this cancellation will be a no-op.

@FinleyMcIlwaine FinleyMcIlwaine marked this pull request as draft August 23, 2024 17:22
@FinleyMcIlwaine FinleyMcIlwaine force-pushed the finley/209 branch 3 times, most recently from 71c1fc8 to 81bb068 Compare August 28, 2024 17:35
@edsko
Copy link
Collaborator

edsko commented Aug 29, 2024

We need to make that when we merge this, we close #209 and consider all mentions of #209 in the code (including the new mention added in #210).

@FinleyMcIlwaine FinleyMcIlwaine changed the title DRAFT: Explicit cancellation of streams on client exceptions Explicit cancellation of streams on client exceptions Aug 30, 2024
@FinleyMcIlwaine FinleyMcIlwaine force-pushed the finley/209 branch 3 times, most recently from 9196096 to ce75d11 Compare August 30, 2024 19:18
@FinleyMcIlwaine FinleyMcIlwaine marked this pull request as ready for review September 1, 2024 23:56
@FinleyMcIlwaine
Explicitly cancel streams on closeRPC
ab9689a 
Using the new `outBodyCancel`, the client can now clearly indicate that it is
closing the stream as the result of an exception, and the server can handle it
properly.
@FinleyMcIlwaine
Copy link
Collaborator Author

I believe this is ready for merge.

As far as the writeChunkFinal goes in the NoMoreElems case of sendMessageLoop, I was able trigger a RST_STREAM frame when it was removed and there were appropriate threadDelays in place. It's difficult to add a test detecting that behavior, since it's very race-y on the http2 side (the finished almost always beats the outBodyCancel resulting from the cancelRequest in grapesy, so we need that threadDelay in the finished to reliably trigger it)

edsko
edsko approved these changes Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@edsko edsko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a relief to get this in!

@edsko edsko merged commit 5f7b140 into main Sep 5, 2024
12 checks passed
@edsko edsko deleted the finley/209 branch September 5, 2024 14:28
@edsko edsko mentioned this pull request Sep 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edsko edsko edsko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FinleyMcIlwaine @edsko